Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance #85802

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance #85802

merged 4 commits into from
Jun 17, 2021

Conversation

Thomasdezeeuw
Copy link
Contributor

Also changes the signature of advance_slice to accept a &mut &mut [IoSlice], not returning anything. This will better match the IoSlice::advance function.

Updates #62726.

@Thomasdezeeuw
Rename IoSlice(Mut)::advance to advance_slice
3803c09 
To make way for a new IoSlice(Mut)::advance function that advances a
single slice.

Also changes the signature to accept a `&mut &mut [IoSlice]`, not
returning anything. This will better match the future IoSlice::advance
function.
@Thomasdezeeuw
Add IoSlice(Mut)::advance
49e25b5 
Advance the internal cursor of a single slice.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021
@Thomasdezeeuw Thomasdezeeuw mentioned this pull request May 29, 2021
Closed
3 tasks
@Mark-Simulacrum
Copy link
Member

r? @m-ou-se but looks okay to me at a cursory look (I have little context on these API changes though)

@Thomasdezeeuw
Copy link
Contributor Author

@Nemo157, @seanmonstar, @mpdn, @Ekleog you've shown interested in #62726, perhaps one of you can give a review?

@Ekleog
Copy link

Ekleog commented Jun 4, 2021

@Thomasdezeeuw I'm sorry, but as of right now I don't really see the point in this API change, so I wouldn't be the right person to ask about that.

As far as I can tell, the slice.advance(n) function you add is basically slice = IoSlice::new(&slice[n..]), except in very specific situations, like the one that raises the need for the function you rename into advance_slices. Maybe giving a concrete example of a time you needed this new advance function in a way where storing an [IoSlice; 1] and using the array-based advance would be too boilerplate-y would help other people like me understand what problem this new function is trying to solve?

As for the &mut -> &mut vs. &mut &mut change, as per my stance in #62726 I can't think of any case where it would have a significant impact, so whichever gets us fastest to stabilization is good to me :)

@Thomasdezeeuw
Copy link
Contributor Author

@Thomasdezeeuw I'm sorry, but as of right now I don't really see the point in this API change, so I wouldn't be the right person to ask about that.

As far as I can tell, the slice.advance(n) function you add is basically slice = IoSlice::new(&slice[n..]), except in very specific situations, like the one that raises the need for the function you rename into advance_slices.

I don't remember the exact reason, but when using IoSliceMut at times this didn't work due to compiler not understand we're essentially reusing the lifetime. Then you needed to go through an awkward mem::replace(&mut buf, &[]) step. But like I said I don't quire remember the details.

Maybe giving a concrete example of a time you needed this new advance function in a way where storing an [IoSlice; 1] and using the array-based advance would be too boilerplate-y would help other people like me understand what problem this new function is trying to solve?

As for the &mut -> &mut vs. &mut &mut change, as per my stance in #62726 I can't think of any case where it would have a significant impact, so whichever gets us fastest to stabilization is good to me :)

@Ekleog
Copy link

Ekleog commented Jun 5, 2021

Is it possible that this memory is the one of implementing the function you're renaming into advance_slices, or are you sure you saw it happen in a different setup? Given it's exactly the way you did it the first time around, and is also the only instance I have personally met, I'm wondering if this vague recollection couldn't maybe match this. Especially as I can't think of a case where mem::replace(_, &mut []) would be useful from outside the stdlib on an IoSlice[Mut], as the inner slice isn't exposed.

Sorry if this question is completely wrong, it's totally possible I'm just failing to see the right use!

@Thomasdezeeuw
Copy link
Contributor Author

@Ekleog I'm not 100% sure anymore. There is https://github.com/rust-lang/futures-rs/blob/e21b5151a1dee1e53c792d2d5bee583633c6699d/futures-util/src/io/write_all_vectored.rs#L37, which uses mem::take (same as mem::replace(bufs, &mut [])), but that uses advance_slices (or would use).

I'm also fine with removing advance, keeping the advance_slices name in case we do ever want the single slice function.

@Thomasdezeeuw
Copy link
Contributor Author

@m-ou-se do you have time to review this, or should I try to assign someone else?

@m-ou-se m-ou-se added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 17, 2021
m-ou-se
m-ou-se approved these changes Jun 17, 2021
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. This looks good to me! Just two typos:

library/std/src/io/mod.rs Outdated Show resolved Hide resolved
library/std/src/io/mod.rs Outdated Show resolved Hide resolved
@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2021

📌 Commit 5e7a8c6 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2021
@JohnTitor JohnTitor mentioned this pull request Jun 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2021
@bors
Auto merge of rust-lang#86399 - JohnTitor:rollup-qlm2dvz, r=JohnTitor
4d3ce2e 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#85663 (Document Arc::from)
 - rust-lang#85802 (Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance)
 - rust-lang#85970 (Remove methods under Implementors on trait pages)
 - rust-lang#86340 (Use better error message for hard errors in CTFE)
 - rust-lang#86343 (Do not emit invalid suggestions on multiple mutable borrow errors)
 - rust-lang#86355 (Remove invalid suggestions for assoc consts on placeholder type error)
 - rust-lang#86389 (Make `sum()` and `product()` documentation hyperlinks refer to `Iterator` methods.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 31ee680 into rust-lang:master Jun 17, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 17, 2021
@Thomasdezeeuw Thomasdezeeuw deleted the ioslice-advance branch June 17, 2021 19:03
@Thomasdezeeuw
Copy link
Contributor Author

Thanks @m-ou-se

@taiki-e taiki-e mentioned this pull request Jun 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmonstar seanmonstar seanmonstar left review comments

@m-ou-se m-ou-se m-ou-se approved these changes

Assignees

@m-ou-se m-ou-se

Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Thomasdezeeuw @rust-highfive @Mark-Simulacrum @Ekleog @m-ou-se @bors @seanmonstar @rustbot